-
Notifications
You must be signed in to change notification settings - Fork 29.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
doc: fix spelling of API name in 10.0.0 changelog #20257
Conversation
as long as there's already a PR open, can you also change |
DOH! sigh ...lol |
@devsnek Is this correct? |
+1 to fast-tracking |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM +1 to fast track
Landed in ad5307f. |
PR-URL: #20257 Refs: #19403 Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Gus Caplan <[email protected]> Reviewed-By: Michael Dawson <[email protected]>
@@ -58,7 +58,7 @@ | |||
* EventEmitter | |||
* The `EventEmitter.prototype.off()` method has been added as an alias for `EventEmitter.prototype.removeListener()`. [[`3bb6f07d52`](https://github.com/nodejs/node/commit/3bb6f07d52)] | |||
* File System | |||
* The `fs.promises` API provides experimental promisified versions of the `fs` functions. [[`329fc78e49`](https://github.com/nodejs/node/commit/329fc78e49)] | |||
* The `fs/promises` API provides experimental promisified versions of the `fs` functions. [[`329fc78e49`](https://github.com/nodejs/node/commit/329fc78e49)] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change is actually wrong. Originally it did indeed land as fs.promises
. I would have kept it as is therefore. There should be another commit that changed it later on.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A good alternative would be to add the commit that changed it to fs/promises
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Feel free to do either, I simply relied on @devsnek's and others reviews.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But this isn't a commit message. This is a textual list of notable changes. Unless v10.0.0 shipped with fs.promises rather than fs/promises, this change is correct, IMO. Later on, when the commits are all listed, it says fs.promises
and that is correct for the reason you identify here. But this section should not be true to specific commits but instead explain the notable change in the release.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This does raise an interesting question though, which is:
Is it necessary to list the commits with the notable changes?
Perhaps it is helpful in some ways, but perhaps it is also misleading. A notable change will often receive subsequent modifications after initially landing. We are not doing the end user favors by providing information in "Notable changes" that describes a change in a form that it was never actually released.
Might this entry in particular be better with no link to a commit and maybe a link to relevant docs about the feature instead?
People reading release notes rarely want to look at source changes, I imagine. Are we making the mistake of designing these release notes for us and not for the end user? Do we need Release Notes that are separate from the ChangeLog and the ChangeLog should be more dry listings and less text? (I don't know the answer here, and this is probably not the place to discuss it, but if someone wants to discuss this in the Release repo or discussion board, maybe that's good?)
I would like to rebase this commit out and remove the change that I pointed out. |
I believe the change is correct as is, as explained in #20257 (comment). |
PR-URL: #20257 Refs: #19403 Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Gus Caplan <[email protected]> Reviewed-By: Michael Dawson <[email protected]>
This is a trivial change (
convertNONProtocols
→convertNPNProtocols
).Refs: #19403
Checklist